-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Split Bundle and StaticBundle
#19761
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. The parallel contents of code for StaticBundle vs Bundle is kinda annoying, but I know that's changing in the next prs, so I'm not worried about it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some small notes on docs that I think we should clean up quick. I would really appreciate a paragraph or two of module docs laying out these traits and how they interconnect, but that's fine to leave to follow-up if you prefer.
More importantly, I'm not sure why we should keep a split between DynamicBundle and Bundle around after these changes. It looks like Bundle should now be object-safe, allowing us to merge them together.
I haven't done a deep dive on this area of the code in a while though, so please feel free to explain why we can't / shouldn't do that :)
However even then it will be a bit of a hassle to remove |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Bundles always felt kind of 'messy' to me, and this direction feels tidier.
4693305 to
f679bac
Compare
Objective
Optionbundles andBox<dyn Bundle>#19491Bundleinto one type representing a static set of component types (StaticBundle) and one that will be used for representing a possibly dynamic set of components.Solution
StaticBundletype, mirroringBundlefor nowBundle(i.e. from its type) to useStaticBundle&selfparameters toBundleto ensure all APIs that use it have a concrete instance to work with in the future.Additional notes
For now this doesn't do anything functionally, as
Bundlestill requires the components to be statically fixed. However this opens the doors for changes to theBundletrait that allow dynamic components without impacting APIs that actually rely on static components.